-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dbg: add dbg_printf_id #946
Conversation
is unsigned 32-bit generic enough for all cases ? Perhaps "string" is an option ? In the past when I was debugging a server with hundreds of calls, we had a |
The problem I see with a string is that this is maybe harder to handle, the basic idea I have in mind is to add setter functions to relevant objects: struct icem {
...
uint32_t debug_id;
}
void icem_set_debug_id(uint32_t debug_id);
static struct ice_candpair *construct_valid_pair(struct icem *icem,
struct ice_candpair *cp,
const struct sa *mapped,
const struct sa *dest)
{
uint32_t debug_id = icem->debug_id;
...
if (!lcand) {
DEBUG_WARNING_ID("no such local candidate: %J\n", mapped);
return NULL;
}
if (!rcand) {
DEBUG_WARNING_ID("no such remote candidate: %J\n", dest);
return NULL;
}
...
} Maybe a fixed length could work? #define DEBUG_ID_LEN 8;
struct icem {
...
char debug_id[DEBUG_ID_LEN];
}
void icem_set_debug_id(char *debug_id);
static struct ice_candpair *construct_valid_pair(struct icem *icem,
struct ice_candpair *cp,
const struct sa *mapped,
const struct sa *dest)
{
char *debug_id = icem->debug_id;
...
if (!lcand) {
DEBUG_WARNING_ID("no such local candidate: %J\n", mapped);
return NULL;
}
if (!rcand) {
DEBUG_WARNING_ID("no such remote candidate: %J\n", dest);
return NULL;
}
...
} |
there is also an option to implement this without changes in DBG module. You can define a session-id string, for example SIP Call-ID or webrtc session-id. Each module, for example ICE, will add this string to all debug messages. When you session is finished, you can take the logfile and grep for the session-id. |
One benefit is with |
I refactored the PR with |
dbg_printf(DBG_WARNING, DEBUG_MODULE ": " __VA_ARGS__) | ||
#define DEBUG_WARNING_ID(...) \ | ||
dbg_printf_id(debug_id, DBG_WARNING, DEBUG_MODULE ": " __VA_ARGS__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it better to pass "debug_id" via the macro parameters, instead of assume the name ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It follows the same technique used for DEBUG_LEVEL and DEBUG_MODULE.
The compiler errors if debug_id
is not defined. I think it's much easier for rewriting existing DEBUG code since you have only define debug_id
and add _ID
. And this needs less reformatting new lines, that was the idea behind it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider uppercase: DEBUG_ID
?
as an example usecase, this ID could be used by baresip/src/call.c with multiple calls, each call has a unique ID. Perhaps also consider options to turn it on/off ? |
{ | ||
(void)level; | ||
(void)arg; | ||
(void)id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the id (aka "tag") be part of the log line ?
46eed9a
to
c6e302b
Compare
@@ -130,7 +130,7 @@ void dbg_handler_set(dbg_print_h *ph, void *arg) | |||
|
|||
|
|||
/* NOTE: This function should not allocate memory */ | |||
static void dbg_vprintf(int level, const char *fmt, va_list ap) | |||
static void dbg_vprintf(struct pl *id, int level, const char *fmt, va_list ap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
for id ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a custom log handler should able to call mem_ref(id)
, this discards the const qualifier.
Should we merge this one to main, or close it ? |
this can probably be merged now Consider the name "ID" or "TAG" for your change. Another thing we should fix is to avoid double locking and formatting here: void dbg_printf(int level, const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
dbg_vprintf(level, fmt, ap);
va_end(ap);
va_start(ap, fmt);
dbg_fmt_vprintf(level, fmt, ap);
va_end(ap);
} |
No description provided.